Skip to content

fix(libkernel): follow a Windows symbolic link in chmod - #9487

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
Gruzzberry:fix/chmod-symbolic-link
Oct 25, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
Gruzzberry:fix/chmod-symbolic-link

Conversation

@Gruzzberry

@Gruzzberry Gruzzberry commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What

On Windows, chmod through a symbolic link changed the link and left the file it names unchanged, and a dangling link succeeded. _wchmod sets the read-only attribute of the path it is given without following a link. FreeBSD 11.4's sys_chmod calls kern_fchmodat with flag 0, which looks the path up with FOLLOW.

NativeChmod now opens a reparse point without FILE_FLAG_OPEN_REPARSE_POINT (so Windows follows it), takes the target's path from the handle and changes that path. A failed open goes through WindowsOpenError_nid_no_patch: ENOENT for a missing target, ELOOP for a loop. The handle-to-path code moves out of WindowsDescriptorPath_nid_no_patch into HandlePath so both use it; fchmod goes through the same descriptor path as before. Paths that are not reparse points take the same _wchmod call as on main. fchmodat with AT_SYMLINK_NOFOLLOW returns before reaching chmod and is unchanged. Linux is unchanged.

Tested

Windows 11 (10.0.26200), WinLibs MinGW-w64 GCC 15.2.0 r7, Ninja, Release, -DBUILD_TESTING=ON, on main d6eafa5 (rebased), Developer Mode on so links can be created.

Overlaps

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); no silent stubs added
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source: none added
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

AI-assisted (Claude Code).

🤖 Generated with Claude Code

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the Windows chmod path and its focused link cases. The non-link path retains its existing _wchmod behavior; for a reparse point the code opens the target (without OPEN_REPARSE_POINT), resolves the handle to the target path, records that target as written, and then changes its mode. The resulting error path uses the repository Windows error mapper, which maps ERROR_CANT_RESOLVE_FILENAME to ELOOP and missing paths to ENOENT. The tests cover changing permissions through a link, ensuring the link itself stays unchanged on Windows, a dangling target, and a loop; they clean up the links afterward. I checked the neighboring open filesystem PRs #9469 and #9474: they touch separate Stdio.cpp regions and the same test file at separate cases, with no overlapping behavior that needs a Depends on relationship. Conventions, intake, eligibility, labels and progress-report checks pass. The automatic product build jobs were skipped; the PR reports Windows focused testing locally.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 5021 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 6936 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 7121 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 7201 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 7447 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 8520 core/libs/prx/libkernel/File/src/Stdio.cpp
core/libs/tests/GuestFilesystem.cpp
PR 9284 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9293 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9297 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9302 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9371 core/libs/tests/GuestFilesystem.cpp
PR 9732 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9823 core/libs/prx/libkernel/File/src/Stdio.cpp

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the Windows path and regression cases. The non-reparse path retains _wchmod; a reparse point is opened following its target, resolved to the target path, recorded as the written path, and then chmodded. The error mapping covers a missing target and a loop. Tests cover changing the target through the symlink, checking the link's own attributes, dangling and loop cases, and cleanup. I manually checked the reported shared Stdio.cpp warnings (#5021, #6879, #6936 and #7201); none changes the chmod or handle-path helpers. Hosted eligibility, intake, conventions, labels and progress-report checks pass; product/relinker jobs are skipped. The author reports focused Windows testing; I did not run local tests.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The chmod path appears correct for the cases reported: non-reparse paths keep _wchmod, a reparse point is followed through a handle-derived target path, and tests cover target mode changes, dangling targets, loops, and cleanup. The prior manual review found the #5021/#6879/#6936/#7201 Stdio warnings outside the chmod/helper path, and #6044’s GuestFilesystem changes were in separate cases. The current branch and Windows tests are based on a6dcea9, while upstream main is ab347d1. Please rebase onto current main, rerun the focused guest_filesystem checks and conventions, then refresh overlap/manual comparisons. I could not retrieve the path-filtered PR patches for the warned PRs in this pass, so I am relying on the earlier review’s manual diff; do not treat this message as a fresh independent diff verification. Hosted product jobs are skipped; I did not run local tests.

@Gruzzberry
Gruzzberry force-pushed the fix/chmod-symbolic-link branch from 74d4c1a to 3fe0d9b Compare October 22, 2026 06:43
@Gruzzberry Gruzzberry mentioned this pull request Oct 22, 2026
12 tasks done
@Gruzzberry

Copy link
Copy Markdown
Contributor Author

Rebased onto main d6eafa5. guest_filesystem and guest_kernel_sys pass on the new head, the new checks fail on that main without the change, and check_conventions.py passes. Full suite on main d6eafa5 and on main with #9442, #9469, #9474, #9487 and #9496 merged together: 1890 tests each, identical results. The overlap scan of the rebased head and a manual check of the pull requests it could not compare are in the description under Overlaps.

@Gruzzberry

Copy link
Copy Markdown
Contributor Author

Holding this as a draft, following the direction on #5239: the open queue is above 700 and no named title needs this change on current main. I will only mark it ready again with a current-main title ID, its Game bring-up discussion and the log line where it stops on this, or once the queue is below 700. Thanks for the reviews so far; the requested fixes are being made on the branch in the meantime.

_wchmod changes the read-only attribute of the path it is given, so on
Windows chmod through a symbolic link changed the link and left the file
it names unchanged, and a dangling link succeeded. A reparse point is now
opened through its target and the target's path is changed, as FreeBSD
11.4's sys_chmod passes FOLLOW: a missing target fails with ENOENT and a
link loop with ELOOP. The path lookup of a handle moves out of
WindowsDescriptorPath_nid_no_patch so both use it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Gruzzberry
Gruzzberry force-pushed the fix/chmod-symbolic-link branch from 3fe0d9b to 4e5db13 Compare October 23, 2026 14:17
@Gruzzberry
Gruzzberry marked this pull request as ready for review October 23, 2026 14:18
@Gruzzberry

Copy link
Copy Markdown
Contributor Author

Rebased onto main 938812d and marked ready for review, as @MarcZX10 noted the #5239 hold does not need to apply here. On Windows (MinGW GCC 15.2.0): the focused tests pass, check_conventions.py passes, and the full suite (2029 tests) on main merged with #9442, #9469, #9474, #9487, #9496, #9661 and #9682 matches main itself, apart from parallel-run flakes that pass when run alone.

@adriadam10 adriadam10 self-assigned this Oct 25, 2026
@adriadam10
adriadam10 dismissed MarcZX10’s stale review October 25, 2026 14:59

Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.

@adriadam10
adriadam10 merged commit 2e1c722 into boykopovar:main Oct 25, 2026
10 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the reparse-point path against FreeBSD's kern_fchmodat (FOLLOW): the target is opened through the handle, HandlePath resolves it, and non-link paths keep the same _wchmod call. Error mapping (ENOENT for a dangling link, ELOOP for a loop) goes through WindowsOpenError_nid_no_patch, and the new guest_filesystem cases cover the target change, the link's own attributes, dangling and loop; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants